Allow SIP plugin to bind media to a specific address (see #3248) #3263

pawnnail · 2023-08-22T06:00:30Z

The multi-stream version of the SIP plugin no longer binds RTP/RTCP sockets to a specific address just like the single-stream version used to. Instead, the multi-stream SIP plugin binds to "any" address for IPv4 and IPv6 (if supported). This highlights a potential security risk if deployed on a multihomed host in a hostile environment. Perhaps, is also wasteful as UDP ports are allocated on multiple addresses and a change of behaviour when migrating from a single-stream to multi. The change was added by PR #3159.

I would like the SIP plugin to use a specific address, IPv4 or IPV6, for media (RTP/RTCP) while preserving the option of binding to any interface (PR #3159). The binding address changes driven by the value of configuration parameter local_media_ip are summarized below.

local_media_ip	single-stream	multi-stream	multi-stream proposed
<empty string>	not supported	not supported	any (IPv4 and IPv6)
<unspecified>	local_ip (IPv4)	any (IPv4 and IPv6)	any (IPv4 and IPv6)
"0.0.0.0"	any (IPv4)	any (IPv4 and IPv6)	any (IPv4 and IPv6)
"::"	not supported	any (IPv4 and IPv6)	any (IPv4 and IPv6)
IPv4, e.g. "192.168.171.8"	192.168.171.8	any (IPv4 and IPv6)	192.168.171.8
IPv4-mapped IPv6, e.g. "::ffff:192.168.171.8"	not supported	any (IPv4 and IPv6)	192.168.171.8
IPVv6, e.g. "2406:da1c:aa9:9401:2825:674b:3e05:fca2"	not supported	any (IPv4 and IPv6)	2406:da1c:aa9:9401:2825:674b:3e05:fca2

So there is an impact on PR #3159: if local_media_ip happens to be specified then RTP/RTCP sockets will bind to that address only rather than to all. There is nothing I can do about it other than to warn. I hope the proposal is flexible enough to address most desired configuration scenarios going forward.

Have tested the above scenarios on Linux and Windows WSL.

Have made sdp_ip used for the SDP connection address header (c=) to fallback to a valid value of either local_media_ip or local_ip.

Also fixed an issue where bind() intermittently fails due to audio_rtp_address and audio_rtcp_address not initialized to 0.

lminiero

Added some notes inline.

lminiero · 2023-08-29T07:47:47Z

src/plugins/janus_sip.c

+	/*
+		Since we might have to derive SDP connection address from local_media_ip make sure it has a meaningful value
+		for the prupose of using it in the SDP c= header.
+	*/


Please don't use this format for comments. Align to what we use in other parts of the code, so something the lines of

/* Since ... * for the ... */

As a side notem there's a typo in "purpose".

The point taken.

The format of the comment is still incorrect.

Sorry about that. Fixed.

lminiero · 2023-08-29T07:49:15Z

src/plugins/janus_sip.c

+	/* Determine the address family to use */
+	if(ipv6_disabled &&
+		!janus_network_address_is_null(&janus_network_local_media_ip) &&
+		janus_network_local_media_ip.family == AF_INET6) {


Code style: you need another tab of indent for the two additional checks above, otherwise it ends up lined with the action below.

Point taken. The condition is removed as redundant.

lminiero · 2023-08-29T07:50:39Z

src/plugins/janus_sip.c

+	if(ipv6_disabled &&
+		!janus_network_address_is_null(&janus_network_local_media_ip) &&
+		janus_network_local_media_ip.family == AF_INET6) {
+		JANUS_LOG(LOG_ERR, "Error setting address for media sockets, IPv6 is disabled and local media address is IPv6...\n");


I don't like this action failing at runtime. It looks like a check that can be done when thew plugin is loaded, that is after the janus_network_local_media_ip property is filled in and we know if ipv6_disabled is FALSE. Better to have the plugin not load at all, if we already know it will never allocate sockets for media.

Makes sense. Moved the failure to the config loading time.

lminiero · 2023-08-29T07:52:29Z

src/plugins/janus_sip.c

 		struct sockaddr_storage audio_rtp_address, audio_rtcp_address;
 		while(session->media.local_audio_rtp_port == 0 || session->media.local_audio_rtcp_port == 0) {
 			if(attempts == 0)	/* Too many failures */
 				return -1;
+			memset(&audio_rtp_address, 0, sizeof(audio_rtp_address));
+			memset(&audio_rtcp_address, 0, sizeof(audio_rtcp_address));
 			if(session->media.audio_rtp_fd == -1) {
 				session->media.audio_rtp_fd = socket(!ipv6_disabled ? AF_INET6 : AF_INET, SOCK_DGRAM, 0);


Did you forget to switch to use_ipv6_address_family here? (and when creating the RTCP socket too, a few lines below). There's other checks that now use use_ipv6_address_family in this code block that should probably move to the other boolean, since it's that one that now dictates the addrlen.

Essentially !ipv6_disabled and use_ipv6_address_family have the same effect, replaced one with the other for clarity.

src/plugins/janus_sip.c

pawnnail · 2023-08-29T20:20:05Z

Thanks for the review, Lorenzo. Will get it sorted within the next few days.

pawnnail · 2023-09-06T21:26:11Z

There are few more things I would like to do to tidy up port binding but not in the same PR. Arbitrary int attempts = 100 tends to fail often on a busy system, especially in a limited port range especially considering RTCP port needs to be adjacent to RTP port. And then it only makes sense to retry bind() on transient errors like hitting a busy port. But one thing at a time.

Retested again. Haven't done the rebase yet, will do once all the changes are approved.
Thanks.

lminiero · 2023-09-07T08:27:39Z

Thanks, looks good now! I just added a tiny note on the comment format.

lminiero · 2023-10-02T14:48:26Z

Apologies for the lack of feedback, I've been pretty busy lately and this is an effort I need to study well, especially considering that, if it works, I'll need to port it to the NoSIP plugin as well, besides backporting it to 0.x for both plugins. I'll try to reserve some time later this week to have a deeper look and make some tests. Scream at me if it doesn't happen 😆

pawnnail · 2023-10-04T01:06:10Z

No problems and thanks. I am not in a hurry. Let me know if I can help.

lminiero · 2023-11-16T10:08:35Z

I'm adapting the patch to 0.x, and then I'll port the changes to NoSIP too. If all goes well, I'll try to merge later today. Apologies again for the delay!

lminiero

@pawnnail while updating other branches and plugins, I stumbled upon these two things. One is a bug and needs to be fixed (it will cause a crash), the other is simply a code style nit. Thanks!

src/plugins/janus_sip.c

lminiero · 2023-11-16T11:36:26Z

src/plugins/janus_sip.c

 			session->media.local_audio_rtp_port = rtp_port;
 			session->media.local_audio_rtcp_port = rtcp_port;
 		}
 	}
 	if(session->media.has_video) {
-		JANUS_LOG(LOG_VERB, "Allocating video ports:\n");
+		JANUS_LOG(LOG_VERB, "Allocating video ports using address [%s]\n",
+			janus_network_address_is_null(&janus_network_local_media_ip  ) ?"any" : local_media_ip);


There are some weird spaces in here. Per the code style, it should be

janus_network_address_is_null(&janus_network_local_media_ip) ? "any" : local_media_ip);

lminiero · 2023-11-16T15:12:18Z

I'll merge and fix the remaining nits myself, since this has been open quite some time. Thanks again for the contribution and the patience!

pawnnail · 2023-11-17T03:52:12Z

Thanks!

This PR contains the following updates: | Package | Update | Change | |---|---|---| | [meetecho/janus-gateway](https://github.com/meetecho/janus-gateway) | minor | `v1.1.4` -> `v1.2.1` | --- ### Release Notes <details> <summary>meetecho/janus-gateway (meetecho/janus-gateway)</summary> ### [`v1.2.1`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v121---2023-12-06) [Compare Source](meetecho/janus-gateway@v1.2.0...v1.2.1) - Added support for abs-capture-time RTP extension \[[PR-3161](meetecho/janus-gateway#3161)] - Fixed truncated label in datachannels (thanks [@veeting](https://github.com/veeting)!) \[[PR-3265](meetecho/janus-gateway#3265)] - Support larger values for SDP attributes (thanks [@petarminchev](https://github.com/petarminchev)!) \[[PR-3282](meetecho/janus-gateway#3282)] - Fixed rare crash in VideoRoom plugin (thanks [@tmatth](https://github.com/tmatth)!) \[[PR-3259](meetecho/janus-gateway#3259)] - Don't create VideoRoom subscriptions to publisher streams with no associated codecs - Added suspend/resume participant API to AudioBridge \[[PR-3301](meetecho/janus-gateway#3301)] - Fixed rare crash in AudioBridge - Fixed rare crash in Streaming plugin when doing ICE restarts \[[Issue-3288](meetecho/janus-gateway#3288)] - Allow SIP and NoSIP plugins to bind media to a specific address (thanks [@pawnnail](https://github.com/pawnnail)!) \[[PR-3263](meetecho/janus-gateway#3263)] - Removed advertised support for SIP UPDATE in SIP plugin - Parse RFC2833 DTMF to custom events in SIP plugin (thanks [@ywmoyue](https://github.com/ywmoyue)!) \[[PR-3280](meetecho/janus-gateway#3280)] - Fixed missing Contact header in some dialogs in SIP plugin (thanks [@ycherniavskyi](https://github.com/ycherniavskyi)!) \[[PR-3286](meetecho/janus-gateway#3286)] - Properly set mid when notifying about ended tracks in janus.js - Fixed broken restamping in janus-pp-rec - Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!) ### [`v1.2.0`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v120---2023-08-09) [Compare Source](meetecho/janus-gateway@v1.1.4...v1.2.0) - Added support for VP9/AV1 simulcast, and fixed broken AV1/SVC support \[[PR-3218](meetecho/janus-gateway#3218)] - Fixed RTCP out quality stats \[[PR-3228](meetecho/janus-gateway#3228)] - Default link quality stats to 100 - Added support for ICE consent freshness \[[PR-3234](meetecho/janus-gateway#3234)] - Fixed rare race condition in VideoRoom \[[PR-3219](meetecho/janus-gateway#3219)] \[[PR-3247](meetecho/janus-gateway#3247)] - Use speexdsp's jitter buffer in the AudioBridge \[[PR-3233](meetecho/janus-gateway#3233)] - Fixed crash in Streaming plugin on mountpoints with too many streams \[[Issue-3225](meetecho/janus-gateway#3225)] - Support for batched configure requests in Streaming plugin (thanks [@petarminchev](https://github.com/petarminchev)!) \[[PR-3239](meetecho/janus-gateway#3239)] - Fixed rare deadlock in Streamin plugin \[[PR-3250](meetecho/janus-gateway#3250)] - Fix simulated leave message for longer string ID rooms in TextRoom (thanks [@adnanel](https://github.com/adnanel)!) [PR-3243](meetecho/janus-gateway#3243)] - Notify about count of sessions, handles and PeerConnections on a regular basis, when event handlers are enabled \[[PR-3221](meetecho/janus-gateway#3221)] - Fixed broken Insertable Streams for recvonly streams when answering in janus.js - Added background selector and blur support to Virtual Background demo - Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.walbeck.it/walbeck-it/docker-janus-gateway/pulls/122 Co-authored-by: renovate-bot <[email protected]> Co-committed-by: renovate-bot <[email protected]>

Allow SIP plugin to bind media to a specific address (see meetecho#3248)

18ea060

lminiero reviewed Aug 29, 2023

View reviewed changes

pawnnail added 3 commits September 7, 2023 08:29

Merge branch 'meetecho:master' into sip-local-media-address

67af7b4

Minor corrections after review.

f3fc5d8

Fail SIP load plugin if ipv6 is wanted but not supported

c3ce78c

pawnnail added 2 commits September 11, 2023 09:52

Comment style fix

fa705e1

Merge branch 'meetecho:master' into sip-local-media-address

2705df7

lminiero added the multistream Related to Janus 1.x label Oct 2, 2023

lminiero reviewed Nov 16, 2023

View reviewed changes

lminiero merged commit d9a5853 into meetecho:master Nov 16, 2023
8 checks passed

lminiero added a commit that referenced this pull request Nov 16, 2023

Small tweaks to #3263, and updated NoSIP plugin with the same changes

f74b313

pawnnail deleted the sip-local-media-address branch November 17, 2023 03:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow SIP plugin to bind media to a specific address (see #3248) #3263

Allow SIP plugin to bind media to a specific address (see #3248) #3263

pawnnail commented Aug 22, 2023

lminiero left a comment

lminiero Aug 29, 2023

pawnnail Sep 6, 2023

lminiero Sep 7, 2023

pawnnail Sep 10, 2023

lminiero Aug 29, 2023

pawnnail Sep 6, 2023

lminiero Aug 29, 2023

pawnnail Sep 6, 2023

lminiero Aug 29, 2023 •

edited

Loading

pawnnail Sep 6, 2023

pawnnail commented Aug 29, 2023

pawnnail commented Sep 6, 2023 •

edited

Loading

lminiero commented Sep 7, 2023

lminiero commented Oct 2, 2023

pawnnail commented Oct 4, 2023

lminiero commented Nov 16, 2023

lminiero left a comment

lminiero Nov 16, 2023

lminiero commented Nov 16, 2023

pawnnail commented Nov 17, 2023

Allow SIP plugin to bind media to a specific address (see #3248) #3263

Allow SIP plugin to bind media to a specific address (see #3248) #3263

Conversation

pawnnail commented Aug 22, 2023

lminiero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lminiero Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawnnail commented Aug 29, 2023

pawnnail commented Sep 6, 2023 • edited Loading

lminiero commented Sep 7, 2023

lminiero commented Oct 2, 2023

pawnnail commented Oct 4, 2023

lminiero commented Nov 16, 2023

lminiero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lminiero commented Nov 16, 2023

pawnnail commented Nov 17, 2023

lminiero Aug 29, 2023 •

edited

Loading

pawnnail commented Sep 6, 2023 •

edited

Loading